Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spiderpool-agent: support to configure the sysctl config for node #3772

Merged

Conversation

cyclinder
Copy link
Collaborator

@cyclinder cyclinder commented Jul 25, 2024

Thanks for contributing!

What type of PR is this?

  • release/feature

What this PR does / why we need it:

support to configure the sysctl config for node

Which issue(s) this PR fixes:

Fixes #3587

Special notes for your reviewer:

{"level":"DEBUG","ts":"2024-07-28T03:50:07.197Z","logger":"spiderpool-agent","caller":"cmd/daemon.go:447","msg":"Setup sysctl","sysctl":"net.ipv4.neigh.default.gc_thresh3","value":"8192"}
{"level":"WARN","ts":"2024-07-28T03:50:07.197Z","logger":"spiderpool-agent","caller":"cmd/daemon.go:458","msg":"skip to setup sysctl","sysctl":"net.ipv4.neigh.default.gc_thresh3","value":"8192","error":"stat /proc/sys/net/ipv4/neigh/default/gc_thresh3: no such file or directory"}
{"level":"DEBUG","ts":"2024-07-28T03:50:07.197Z","logger":"spiderpool-agent","caller":"cmd/daemon.go:447","msg":"Setup sysctl","sysctl":"net.ipv6.neigh.default.gc_thresh3","value":"8192"}
{"level":"WARN","ts":"2024-07-28T03:50:07.197Z","logger":"spiderpool-agent","caller":"cmd/daemon.go:458","msg":"skip to setup sysctl","sysctl":"net.ipv6.neigh.default.gc_thresh3","value":"8192","error":"stat /proc/sys/net/ipv6/neigh/default/gc_thresh3: no such file or directory"}
{"level":"DEBUG","ts":"2024-07-28T03:50:07.197Z","logger":"spiderpool-agent","caller":"cmd/daemon.go:447","msg":"Setup sysctl","sysctl":"net.ipv4.conf.all.arp_notify","value":"1"}
{"level":"DEBUG","ts":"2024-07-28T03:50:07.197Z","logger":"spiderpool-agent","caller":"cmd/daemon.go:450","msg":"success to setup sysctl","sysctl":"net.ipv4.conf.all.arp_notify","value":"1"}
{"level":"DEBUG","ts":"2024-07-28T03:50:07.197Z","logger":"spiderpool-agent","caller":"cmd/daemon.go:447","msg":"Setup sysctl","sysctl":"net.ipv4.conf.all.forwarding","value":"1"}
{"level":"DEBUG","ts":"2024-07-28T03:50:07.197Z","logger":"spiderpool-agent","caller":"cmd/daemon.go:450","msg":"success to setup sysctl","sysctl":"net.ipv4.conf.all.forwarding","value":"1"}
{"level":"DEBUG","ts":"2024-07-28T03:50:07.197Z","logger":"spiderpool-agent","caller":"cmd/daemon.go:447","msg":"Setup sysctl","sysctl":"net.ipv6.conf.all.forwarding","value":"1"}
{"level":"DEBUG","ts":"2024-07-28T03:50:07.197Z","logger":"spiderpool-agent","caller":"cmd/daemon.go:450","msg":"success to setup sysctl","sysctl":"net.ipv6.conf.all.forwarding","value":"1"}

@cyclinder cyclinder added cherrypick-release-v0.9 cherrypick-release-v1.0 Cherry-pick the PR to branch release-v1.0. labels Jul 25, 2024
Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.16%. Comparing base (cfae10b) to head (8e64eee).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3772      +/-   ##
==========================================
- Coverage   81.21%   81.16%   -0.05%     
==========================================
  Files          50       50              
  Lines        4391     4391              
==========================================
- Hits         3566     3564       -2     
- Misses        669      670       +1     
- Partials      156      157       +1     
Flag Coverage Δ
unittests 81.16% <ø> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

@cyclinder cyclinder force-pushed the spiderpoolagent/opt_sysctl branch 2 times, most recently from 2eef7b8 to f1b44f1 Compare July 26, 2024 05:48
@cyclinder cyclinder added the release/feature-new release note for new feature label Jul 26, 2024
@cyclinder cyclinder force-pushed the spiderpoolagent/opt_sysctl branch 3 times, most recently from fdd4301 to 0375792 Compare July 26, 2024 08:13
@@ -454,6 +454,9 @@ spiderpoolAgent:
securityContext: {}
# runAsUser: 0

## @param spiderpoolAgent.sysctlConfigs the sysctl configs of spiderpoolAgent pod
Copy link
Collaborator

@weizhoublue weizhoublue Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我在想,我们是否不提供该接口,spiderpool 不负责 sysctl 的其他职责,只是确保自身正常工作
只需要提供一个 disable tune sysctl 的参数即可,避免我们的默认值 不符合预期。
其它的需求,用户自己去设置 主机的 sysctl 写配置文件

spiderpool 不承担 其它 sysctl 值的 调优职责,也避免 用户 通过该 接口 设置了一些 sysctl,把 spiderpool 的相关代码 跑挂了,我们是 退出好,还是 忽略好 ,都挺麻烦

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我们可以保留这个接口,但不推荐用户使用此方式来来改任何 sysctl, 我们只是用它来做为我们自己的后门,方便以后要是出问题的时候,我们可以通过它快速配一下,避免需要重新编写代码

Value: "8192",
},
{
Name: "net.ipv6.neigh.default.gc_thresh3",
Copy link
Collaborator

@weizhoublue weizhoublue Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

上一行 comment,什么内核 或 高内核 才有该 ipv6 的值。内核不存在该值 时,会自动忽略

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

没搜到内核版本支持?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

net.ipv6.neigh.default.gc_thresh3 (since Linux 2.2)
但是什么版本被拿掉了,要看下

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

就是没找到,内核代码没明显看出来

// DefaultSysctlConfig is the default sysctl config for the node
var DefaultSysctlConfig = []struct {
Name string
Value string
Copy link
Collaborator

@weizhoublue weizhoublue Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

再加一些 字段 ?
requiredKernelVersion bool 一些低内核下,一些值是不存在的,不需要设置,忽略它们是否设置成功
setOnIpv4 : true Spiderpool 工作在 单双栈等场景下,只设置自己关心的值
setOnipv6: true

或者看其他项目是如何实践的

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kube-proxy 会判断kernel 版本,这个有点复杂了,calico 会通过 是否启用 ipv6 来决定配置 ipv6 的参数

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里如果需要判断 kernel 版本 和 是ipv4 或 ipv6,就有点繁琐了,这里应该是尽量去设置,忽略可接受的失败,否则如 ipv6-only 下还要去判断 ipv4 的 sysctl 不能去设置

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spidepool helm 安装选项 中 有 是否启用 ipv4 还是 ipv6 的 环境变量 传递进来

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

有的

@cyclinder cyclinder force-pushed the spiderpoolagent/opt_sysctl branch 3 times, most recently from de11d60 to 8879bda Compare July 26, 2024 11:22
@cyclinder cyclinder force-pushed the spiderpoolagent/opt_sysctl branch 6 times, most recently from edc0d43 to efacd86 Compare July 28, 2024 07:17
@cyclinder cyclinder force-pushed the spiderpoolagent/opt_sysctl branch 3 times, most recently from 2601752 to 32b8947 Compare July 31, 2024 02:54
| `ipam.spidersubnet.enable` | SpiderSubnet feature. | `true` |
| `ipam.spidersubnet.autoPool.enable` | SpiderSubnet Auto IPPool feature. | `true` |
| `ipam.spidersubnet.autoPool.defaultRedundantIPNumber` | the default redundant IP number of SpiderSubnet feature auto-created IPPools | `1` |
| `ipam.spiderSubnet.enable` | SpiderSubnet feature. | `true` |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

虽然是带货一些小错误,但要动 这些 字段,注意 doc 下一些 文档 要纠正

@@ -124,6 +124,7 @@ helm install spiderpool spiderpool/spiderpool --wait --namespace kube-system \
| `global.ipamUNIXSocketHostPath` | the host path of unix domain socket for ipam plugin | `/var/run/spidernet/spiderpool.sock` |
| `global.configName` | the configmap name | `spiderpool-conf` |
| `global.ciliumConfigMap` | the cilium's configMap, default is kube-system/cilium-config | `kube-system/cilium-config` |
| `global.tuneSysctlConfig` | enable set specify sysctl configs for each node. | `false` |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个就是 spdieragent.tuneSysctlConfig 毕竟合适,它不生效到 其它组件

@@ -245,15 +245,11 @@ spec:
{{- with .Values.spiderpoolAgent.extraEnv }}
{{- toYaml . | nindent 8 }}
{{- end }}
{{- if or .Values.dra.enabled .Values.spiderpoolAgent.securityContext }}
securityContext:
Copy link
Collaborator

@weizhoublue weizhoublue Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个默认是 强制的? 难免有一些 安全风险 和 背锅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

哦,这个应该要恢复


| sysctl config | value | description |
| -------------| ------| ------------|
| net.ipv4.neigh.default.gc_thresh3 | 8196 | This is the hard maximum number of entries to keep in the ARP cache. The garbage collector will always run if there are more than this number of entries in the cache. for ipv4 |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8196 -> 20480
省得又不够

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

文档需要更新

@cyclinder cyclinder force-pushed the spiderpoolagent/opt_sysctl branch 3 times, most recently from 02403bd to 53dfba9 Compare July 31, 2024 07:52
@@ -450,6 +450,9 @@ spiderpoolAgent:
## @param spiderpoolAgent.resources.requests.memory the memory requests of spiderpoolAgent pod
memory: 128Mi

## @param spiderpoolAgent.tuneSysctlConfig enable set specify sysctl configs for each node.
tuneSysctlConfig: false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个默认是 true 把 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我怕打开会覆盖客户的默认值?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? tuneSysctlConfig: true

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这些事 underlay 必要条件,应该是默认的

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -282,6 +282,7 @@ helm install spiderpool spiderpool/spiderpool --wait --namespace kube-system \
| `spiderpoolAgent.resources.limits.memory` | the memory limit of spiderpoolAgent pod | `1024Mi` |
| `spiderpoolAgent.resources.requests.cpu` | the cpu requests of spiderpoolAgent pod | `100m` |
| `spiderpoolAgent.resources.requests.memory` | the memory requests of spiderpoolAgent pod | `128Mi` |
| `spiderpoolAgent.tuneSysctlConfig` | enable set specify sysctl configs for each node. | `false` |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enable to set required sysctl on each node to run spiderpool. refer to http:/.....doc... for details

@@ -75,6 +76,15 @@ func DaemonMain() {
}
logger.Sugar().Infof("Spiderpool-agent config: %+v", agentContext.Cfg)

// setup sysctls
if agentContext.Cfg.TuneSysctlConfig {
if err := sysctlConfig(agentContext.Cfg.EnableIPv4, agentContext.Cfg.EnableIPv6); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

作为 info 级别的日志,打开时,看不到一行 " set syctl " 日志

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个日志是有的

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if _, err := sysctl.Sysctl(sysConfig, value); err != nil {
return err
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

需要一行 info 级别日志: sysctl 什么 key 设置了 什么值

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weizhoublue
weizhoublue previously approved these changes Aug 1, 2024
@cyclinder cyclinder force-pushed the spiderpoolagent/opt_sysctl branch 4 times, most recently from 8bae9e9 to 0ed157f Compare August 5, 2024 02:54
Signed-off-by: cyclinder <qifeng.guo@daocloud.io>
@weizhoublue weizhoublue merged commit fc04adf into spidernet-io:main Aug 6, 2024
57 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 6, 2024
spiderpool-agent: support to configure the sysctl config for node
Signed-off-by: robot <tao.yang@daocloud.io>
cyclinder pushed a commit to cyclinder/spiderpool that referenced this pull request Aug 7, 2024
…pt_sysctl

spiderpool-agent: support to configure the sysctl config for node
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick-release-v0.8 Cherry-pick the PR to branch release-v0.8. cherrypick-release-v0.9 cherrypick-release-v1.0 Cherry-pick the PR to branch release-v1.0. release/feature-new release note for new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

optimize the sysctl
2 participants